Validate planar cell size in slope() (#2897)#2901
Merged
Merged
Conversation
brendancol
commented
Jun 3, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Validate planar cell size in slope() (#2897)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
- slope.py:406-414 The guard uses
cellsize_x <= 0while the sibling guard in curvature.py:256-263 usescellsize_x == 0. The divergence is deliberate: issue #2897 asks slope to reject negative resolutions too, and a negative cell size would silently flip the slope sign. A one-line comment would stop a future reader from "harmonizing" the two guards back to== 0. - test_slope.py:483-520 The four backend-specific parametrized tests are near-identical. Validation runs in the wrapper before dispatch, so the numpy test alone already covers the code path. The cupy/dask variants are cheap insurance that no backend swallows the error, so keeping them is fine; they could be collapsed if the file grows.
What looks good
- The check sits in the public wrapper before
ArrayTypeFunctionMappingdispatch, so all four backends behave the same without per-backend code. Matches the curvature.py precedent. - NaN/inf ordering is correct:
not np.isfinite(...)is evaluated first viaor, so NaN never reaches the<= 0comparison (which is False for NaN and would let it through). - The geodesic path is left untouched, with an explicit test confirming a bogus
resattribute does not trip the planar guard. - The error message names the offending values and says how to fix the resolution, in the curvature.py message style.
- Tests hit zero, negative, NaN, and inf on each axis independently, plus a positive-cellsize regression test.
Checklist
- Algorithm matches reference/paper (no algorithm change; validation only)
- All implemented backends produce consistent results (validation is backend-agnostic, runs in wrapper)
- NaN handling is correct
- Edge cases are covered by tests
- Dask chunk boundaries handled correctly (unchanged; guard runs before dispatch)
- No premature materialization or unnecessary copies
- Benchmark exists or is not needed (validation guard, no perf impact)
- README feature matrix updated (not applicable; no new function or backend change)
- Docstrings present and accurate
brendancol
commented
Jun 3, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
Follow-up review (after f1a5eab)
The single actionable nit from the previous pass is resolved: slope.py now carries a one-line comment explaining why the guard uses <= 0 (rejecting negatives) where curvature() uses == 0. No new code paths, so the earlier checklist still holds.
The second nit (collapsing the four near-identical backend tests) is left as-is on purpose. The cupy and dask variants are cheap and confirm no backend wrapper swallows the error before it reaches the user. Collapsing them would trade that coverage for a few saved lines, which is not worth it here.
No remaining blockers or suggestions.
# Conflicts: # xrspatial/tests/test_slope.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
slope()path, matching the guard already incurvature().slope(method='planar')now raises a clearValueErrorwhen the resolution on either axis is zero, negative, NaN, or infinite. Before this, zero gave a rawZeroDivisionError, NaN gave a silently-NaN interior, and inf gave wrong values that looked plausible.resattribute.Backend coverage
numpy, cupy, dask+numpy, dask+cupy (planar). Validation runs in the wrapper, so all four behave identically.
Test plan
ValueErrorfor zero/negative/NaN/inf cell sizes across all four backends.resattribute and does not trip the planar guard.test_slope.pyandtest_geodesic_slope.pypass (153 tests).Closes #2897